Skip to content

Client API design rev 2.2: heartbeat phases, per-launch tokens, F3 dependencies; add TASK_PAYLOAD_READY#4866

Closed
YuanTingHsieh wants to merge 1 commit into
NVIDIA:mainfrom
YuanTingHsieh:yuantingh/client-api-design-rev22
Closed

Client API design rev 2.2: heartbeat phases, per-launch tokens, F3 dependencies; add TASK_PAYLOAD_READY#4866
YuanTingHsieh wants to merge 1 commit into
NVIDIA:mainfrom
YuanTingHsieh:yuantingh/client-api-design-rev22

Conversation

@YuanTingHsieh

Copy link
Copy Markdown
Collaborator

Design/plan revision 2.2 for Client API Execution Modes (follow-up to #4853), addressing design-review findings, plus the matching protocol-vocabulary follow-up to #4856.

Design (client_api_execution_modes.md):

  • Forward-path heartbeat rule narrowed: the no-expiry exemption now covers only the payload-materialization phase — TASK_ACCEPTED → new TASK_PAYLOAD_READY message — bounded by a materialization deadline; heartbeats stay authoritative while user code trains, so a wedged process is detected at heartbeat timeout, not task timeout. Sequence diagrams updated (all four re-validated).
  • launch_once=False moves to launch-scoped tokens: the CJ regenerates the token in each per-launch bootstrap config and stopping a process invalidates its token — a surviving stale process cannot authenticate against a later launch.
  • Explicit mapping between lifecycle transfer states and the F3 TransferOutcome vocabulary (TRANSFER_COMPLETE ⇔ COMPLETED; TRANSFER_FAILED ⇔ FAILED or ABORTED; receiver truth applies before the mapping).
  • ClientAPIBackendSpec classified internal (frozen for parallel development, not public API in 2.9); Open Questions annotated with their deciding PRs; status set to Approved.

Plan (client_api_execution_modes_plan.md):

  • F3-4 now hard-depends on F3-2 + F3-3 (the facade's returns-means-delivered guarantee holds only over receiver-confirmed, retry-aware outcomes with budget-bounded resolution); critical path updated — F3-2/F3-3 run in parallel behind F3-1, still inside the 10–14-week floor.
  • Bookkeeping: PR-0 recorded as landing inside Add Client API execution modes design docs and F3 aggregate transfer outcome #4853; EX-5 moved to Wave 4 and assigned P1; CT-5 assigned P1; tier accounting made exact (36 PRs; CT-4 halves noted).

Vocabulary (client/cell/defs.py): adds Topic.TASK_PAYLOAD_READY#4856 merged before the phase-boundary message was introduced; the freeze tests are updated accordingly.

🤖 Generated with Claude Code

…s, status

Address design-review findings:

- Forward-path heartbeat rule narrowed: the no-expiry exemption now covers only
  the payload-materialization phase (TASK_ACCEPTED -> new TASK_PAYLOAD_READY
  message), bounded by a materialization deadline; heartbeats stay authoritative
  while user code trains, so a wedged process is detected at heartbeat timeout,
  not task timeout. Diagrams updated (mermaid re-validated).
- launch_once=False moves to launch-scoped tokens: the CJ regenerates the token
  in each per-launch bootstrap config and stopping a process invalidates its
  token, so a surviving stale process cannot authenticate against a later launch.
- Plan dependencies now enforce the design's payload-safety prerequisites:
  F3-4 hard-depends on F3-2 + F3-3 (receiver-confirmed, retry-aware outcomes;
  budget-bounded resolution); critical path updated (F3-2/F3-3 parallel behind
  F3-1, one M added, still inside the 10-14 week floor).
- Explicit mapping between lifecycle transfer states and the F3 TransferOutcome
  vocabulary (TRANSFER_COMPLETE <=> COMPLETED; TRANSFER_FAILED <=> FAILED or
  ABORTED; receiver truth applies before the mapping).
- Bookkeeping: PR-0 recorded as landing inside the F3-1 PR (NVIDIA#4853); EX-5 moved
  to Wave 4 (its EP-4 dependency) and assigned P1; CT-5 assigned P1; tier
  accounting line added (36 PRs, CT-4 halves in P1/P2); P0+P1 ~36 engineer-weeks.
- ClientAPIBackendSpec classified as internal (frozen for parallel development,
  not public API in 2.9). Open Questions annotated with their deciding PRs.
- Status set to Approved; Revision 2.2 entry added.
- Add Topic.TASK_PAYLOAD_READY to the frozen protocol vocabulary (follow-up to
  NVIDIA#4856, which merged before the phase-boundary message was introduced).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 7, 2026 23:57
@greptile-apps

greptile-apps Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates the Client API execution-mode design and protocol vocabulary.

  • Adds the TASK_PAYLOAD_READY client-cell topic.
  • Updates the topic freeze tests for the new wire value.
  • Narrows the heartbeat exemption to the payload-materialization phase.
  • Revises the implementation plan for F3 dependencies, EX-5 timing, and tier accounting.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • The protocol constant is additive and covered by the updated freeze tests.
  • The documentation updates are consistent with the new materialization phase boundary.

Important Files Changed

Filename Overview
nvflare/client/cell/defs.py Adds the TASK_PAYLOAD_READY topic constant to the client-cell protocol vocabulary.
tests/unit_test/client/cell/defs_test.py Updates expected topic names and wire values to include TASK_PAYLOAD_READY.
docs/design/client_api_execution_modes.md Documents the revised heartbeat phase boundary, per-launch token behavior, transfer-outcome mapping, and internal backend-spec status.
docs/design/client_api_execution_modes_plan.md Updates plan sequencing, dependency notes, tier counts, and critical-path estimates.

Reviews (1): Last reviewed commit: "Design/plan updates: heartbeat phases, p..." | Re-trigger Greptile

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the Client API Execution Modes design/plan (rev 2.2) and aligns the frozen Cell protocol vocabulary with the newly introduced TASK_PAYLOAD_READY phase boundary used to narrow the forward-path heartbeat exemption.

Changes:

  • Add Topic.TASK_PAYLOAD_READY to the Client API Cell protocol vocabulary and update freeze tests.
  • Revise the design doc to introduce the payload-materialization phase boundary and per-launch token semantics.
  • Revise the implementation plan to reflect updated dependencies/critical path and tier accounting.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/unit_test/client/cell/defs_test.py Extends protocol freeze tests to include TASK_PAYLOAD_READY.
nvflare/client/cell/defs.py Adds the TASK_PAYLOAD_READY topic constant and documents its intended semantics.
docs/design/client_api_execution_modes.md Design rev 2.2 updates: new phase boundary message, heartbeat rule refinement, token scoping, and outcome mapping details.
docs/design/client_api_execution_modes_plan.md Plan rev 2.2 updates: dependency adjustments (notably F3-4), critical path, and bookkeeping/tier accounting.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 264 to 268
@@ -260,7 +267,7 @@ sequenceDiagram
Note over E,T: WAIT_TRANSFER_COMPLETE (hold process until terminal)
E->>C: result
C->>E: task
E->>T: TASK_READY (task_id, FLModel ref)
T->>E: TASK_ACCEPTED
T->>E: TASK_PAYLOAD_READY (payload pulled)
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.27%. Comparing base (fb6ef21) to head (5a81394).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4866      +/-   ##
==========================================
- Coverage   60.28%   60.27%   -0.02%     
==========================================
  Files         971      971              
  Lines       92463    92464       +1     
==========================================
- Hits        55745    55732      -13     
- Misses      36718    36732      +14     
Flag Coverage Δ
unit-tests 60.27% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@YuanTingHsieh

Copy link
Copy Markdown
Collaborator Author

Folded into #4865 per review consolidation — the design revision and protocol vocabulary land with the F3 code they document.

@YuanTingHsieh YuanTingHsieh deleted the yuantingh/client-api-design-rev22 branch July 8, 2026 01:34
YuanTingHsieh added a commit that referenced this pull request Jul 13, 2026
…, awaitable transfer facade (#4865)

The complete F3/Cell payload layer the Client API execution modes build
on, plus the design revision that documents it. Upper layers (executor
backends, trainer engine) consume one primitive:

```python
waiter = downloader.get_waiter()
outcome = waiter.wait(timeout=...)   # returns == delivered
```

*(This PR ships the primitive and the truth model behind it; production
callers of `get_waiter()` arrive with the trainer-engine and
executor-backend PRs. Nothing user-visible changes yet.)*

### How a huge-model send flows: before and after

**Before this PR — served is assumed delivered.** The sender's books say
SUCCESS when the last chunk leaves; whether the receiver could actually
store the model is unknowable, and the terminal FINISHED status counts
failed receivers too:

```mermaid
sequenceDiagram
    participant S as sender cell (holds 70GB model)
    participant R as receiver cell

    Note over S: flare.send() registers refs, then waits on ad-hoc<br/>machinery: progress messages, timeout guesses, ack grace.
    S->>R: small control message (ref ids, no payload)

    loop receiver-driven chunk pulls
        R->>S: request(R1, state)
        S-->>R: reply(chunk, next state)
    end

    R->>S: request(R1, final state)
    S-->>R: reply(EOF)
    Note over S: sender counts this receiver as SUCCESS<br/>the moment the last chunk is served.

    Note over R: receiver stores the model (e.g. disk write).<br/>If this fails, nobody tells the sender.

    Note over S: transaction FINISHED means receiver COUNT reached,<br/>counting failed receivers too.<br/>No queryable verdict exists afterward.
    Note over S,R: consequences: the sender frees the model or exits its<br/>subprocess on a guess. A receiver whose store failed is<br/>invisible: the workflow proceeds without the model (silent truncation).
```

**With this PR — served ≠ delivered; the receiver's own stored/failed
report decides.** Every receiver's outcome is knowable, bounded in time,
and queryable afterward:

```mermaid
sequenceDiagram
    participant S as sender cell (holds 70GB model)
    participant R as receiver cell

    Note over S: sender registers transaction T0 with refs R1..Rn.<br/>The model stays in sender memory - only refs travel.<br/>waiter = get_transfer_waiter(T0) is the facade this PR adds.
    S->>R: small control message (ref ids, no payload)

    loop receiver-driven chunk pulls (MBs per chunk)
        R->>S: request(R1, state, CONFIRM_CAPABLE)
        S-->>R: reply(chunk, next state)
    end

    R->>S: request(R1, final state, CONFIRM_CAPABLE)
    S-->>R: reply(EOF, CONFIRM_EXPECTED)
    Note over S: sender notes: all bytes served to this receiver,<br/>outcome pending the receiver's confirmation.<br/>Not yet counted as delivered.

    Note over R: receiver stores the assembled model<br/>(download_completed, e.g. write the file to disk).<br/>This step can fail even though every byte arrived.
    R->>S: CONFIRM: stored OK / store failed (fire and forget)

    Note over S: sender records what the receiver reported.<br/>stored OK: counted as delivered.<br/>store failed: counted as failed, despite the served EOF.
    Note over S: when every expected receiver is counted, the monitor closes T0:<br/>callbacks run, 70GB source release attempted (errors logged), THEN the verdict is recorded<br/>(kept 30 min) and waiter.wait() returns - fully settled.

    Note over S,R: bounds on the sender's 5s monitor tick. Budgets are opt-in,<br/>set per transaction or via config vars - acquire needs declared receiver ids.<br/>receiver never pulls: failed at the acquire budget.<br/>receiver goes silent or its CONFIRM is lost: failed at the idle budget.<br/>either budget unset: the transaction TTL remains the backstop.<br/>old-version receiver (no CONFIRM_CAPABLE): counted at EOF, exactly today's behavior.
```

### The trade-off, stated plainly

Holding the source: the sender keeps the model longer **only on the
happy path** (+ receiver store time + one confirm hop) — exactly the
window in which the old code had already freed a model whose delivery
was unproven. In the degraded cases the hold is *shorter* than before:

| Path | Before | After |
|---|---|---|
| Happy path | freed when last chunk **served** | + receiver store +
confirm hop |
| Receiver stalls mid-pull | pinned until full tx TTL | freed at the
**idle budget** (sooner) |
| Receiver never pulls | pinned until TTL | freed at the **acquire
budget** (sooner) |
| Absolute worst case | TTL | TTL (unchanged backstop) |

*The budget rows apply when budgets are configured (opt-in, per
transaction or via `streaming_receiver_acquire_timeout` /
`streaming_receiver_idle_timeout`; the acquire budget additionally
requires declared receiver identities). Unconfigured deployments keep
exactly the before-column TTL behavior.*

Network hiccups: chunk/EOF loss is handled as before (retries, tombstone
replay). The one new must-arrive message is the CONFIRM; if it is lost,
the receiver is counted FAILED despite a good store — a **false
negative**, bounded by the idle budget, healed by an idempotent retry.
This is the Two Generals bound: some message is always last and
unconfirmed, so a protocol only chooses which way it errs. Before, we
erred toward invisible false success (silent truncation); now we err
toward visible, bounded, retryable false failure.
Corruption-by-assumption is no longer a possible outcome.

**Outcome-recording hardening** (follow-up to #4853's F3-1): **tx_ids
are attempt-scoped and single-use while known** — a retry of the same
logical transfer is a new transaction with a new id, and
`new_transaction` rejects a duplicate id (`ValueError`) while it is
live, still settling, its receipt is retained (`TX_OUTCOME_TTL`), or a
drain-timeout leak from a previous attempt is still in flight. The
default fresh uuid never collides; a caller-supplied id becomes reusable
only after its receipt expired AND every old operation exited. The
stable cross-attempt identity is the application-level transfer id
carried in the caller's metadata; it never enters this service.
Uniqueness is what makes every emission of a terminated transaction
attributable to it alone — no suppression flags, generation
serialization, or settlement waiting exist (an earlier revision
serialized reused-id generations; review falsified that boundary
repeatedly, so the boundary was deleted at the naming level). Ownership
(`_outcome_owners`) and `_tx_table` registration remain one atomic step,
so a transaction the monitor can terminate always owns its outcome slot,
concurrent same-id constructors resolve to exactly one winner and clean
errors for the losers, and `shutdown()` clears ownership atomically with
table teardown (nothing records after shutdown; the owner guard alone
gates it). In-flight operations (serves, confirms, monitor budget
passes) register with a per-transaction **activity gate** that
settlement closes and drains before snapshotting the outcome — an
operation finishing during termination is counted in the verdict, and
nothing emits against a settled transaction (only a callback hanging
beyond the 60s drain timeout can leak a late emission, loudly logged —
and such a leak keeps its tx_id excluded from registration until the
operation exits, so a leaked emission can never land under a recycled
id); `tx` required in `_record_outcome` (recording is legal only for the
slot owner); deep-frozen `TransferOutcome` (note: `receiver_statuses` is
a `MappingProxyType` — not JSON/pickle-serializable by design; consumers
crossing a boundary materialize with `dict(...)`); exception-guarded
`downloaded_to_*` callbacks.

**Receiver-confirmed completion + retry-aware accounting**: a
confirm-capable receiver's served EOF/ERROR is provisional; the
receiver's fire-and-forget confirmation — carrying its stored-OK /
store-failed truth (a finalization failure confirms FAILED) — finalizes
it. Receiver truth wins (served-EOF + failed store = FAILED — the
disk-offload case); retries overwrite provisional records. Every
confirmation echoes a **per-serve nonce** from the terminal reply,
binding it to its exact serve: a stale confirmation from a previous life
of a reused ref_id is dropped even when the new life has its own pending
serve for the same receiver. Confirmations ride with the download's
`secure` setting. All wire keys optional (both version skews degrade to
today's producer-served semantics); per-process kill-switch
`streaming_receiver_confirm_enabled` disables the wire behavior on
either side without a code revert.

**Declared receiver identities + per-(transfer, receiver) budgets +
quorum**: when `receiver_ids` are declared, completion, the aggregate
outcome, and the quorum are judged against **those identities** — a
status from an unexpected receiver can never complete a transfer a
declared receiver did not get. Budgets (opt-in): a never-pulling
receiver fails at the acquire budget (requires declared identities);
idleness is judged on **transaction-level** activity, so a receiver that
finished one ref and went silent cannot escape a sibling ref's budgets —
with a truth-wins re-check on enforcement. Budget failures resolve the
outcome on a monitor pass instead of the TTL, which also bounds lost
confirmations (fail-closed). `min_receivers`/`quorum_met` give fan-out
workflows the k-of-N surface (a quorum receiver must be a declared
receiver and succeed on every ref); `completed` stays the strict
all-receivers certificate.

**`TransferWaiter` awaitable facade**: event-driven and
**settle-then-resolve** — the outcome is recorded (and waiters released)
only after the callback chain and the source-release attempts complete
(a raising `release()` is logged and cannot block settlement or strand
the waiter), so acting on `wait()` returning can never preempt them.
Never hangs (unknown/expired/shut-down ids resolve immediately; shutdown
releases all waiters). tx_ids are attempt-scoped, so a waiter always
resolves with the verdict of exactly the attempt it named; a retrying
caller acquires a new waiter for the new attempt. Optional
caller-specified linger (FINISHED-gated) preserves the tombstone replay
window; `acquired_receivers()` is the V1 PAYLOAD_ACQUIRED signal;
composes with — never replaces — the existing `DOWNLOAD_COMPLETE_CB`
chain.

**Design rev 2.2 + vocabulary** (folded from the former #4866):
forward-path heartbeat exemption narrowed to the payload-materialization
phase via new `Topic.TASK_PAYLOAD_READY` (heartbeats stay authoritative
during training; diagrams re-validated); `launch_once=False` moves to
launch-scoped tokens; F3 dependency/vocabulary-mapping updates; status
set to Approved. The implementation-plan doc is removed — it was a
point-in-time PR decomposition that execution has already deviated from;
the design doc is the durable reference.

**Tests**: 60+ new unit tests across `receiver_confirm_test`,
`receiver_budget_test`, `transfer_waiter_test`, and the F3-1 hardening
suite — skew matrix, kill-switch both sides, receiver-truth-wins, retry
healing, mixed fleets, tombstone interplay, unsolicited-confirm guard,
bounded lost-confirm, multi-ref acquisition, quorum intersection, waiter
never-hangs invariants. Full streaming+fobs+legacy sweeps green.
Reviewed by adversarial multi-agent workflows (24 + 6 confirmed findings
fixed) plus a 4-angle cleanup pass (net −48 LOC, hot-path trims).

---------

Co-authored-by: Zhihong Zhang <100308595+nvidianz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants